esm: Ensure custom loader resolved "url" is properly validated #21352

guybedford · 2018-06-15T14:13:54Z

This ensures that any loader returning a non-valid URL from the resolve hook will throw an error in the main resolution.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

nodejs-github-bot · 2018-06-15T14:13:55Z

@guybedford build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/81/pipeline

devsnek · 2018-06-15T14:42:15Z

test/fixtures/es-module-loaders/loader-invalid-url.mjs

@@ -0,0 +1,8 @@
+export async function resolve(specifier, parentModuleURL, defaultResolve) {
+  if (parentModuleURL && specifier !== 'assert')


nit: braces around multi-line condition body

jasnell · 2018-06-15T15:50:18Z

lib/internal/modules/esm/loader.js

+        new URL(url);
+      } catch (err) {
+        throw new ERR_INVALID_URL(url);
+      }


hmmm... I'm not sure what the benefit of the try/catch and additional throw here. If the url is invalid, new URL(url) will throw and accomplish the same thing, right?

Well caught, yes that's simpler. Updated.

guybedford · 2018-06-15T18:16:07Z

CI: https://ci.nodejs.org/job/node-test-pull-request/15484/

TimothyGu · 2018-06-17T04:35:11Z

lib/internal/modules/esm/loader.js

@@ -64,6 +65,11 @@ class Loader {
    if (format === 'builtin')
      return { url: `node:${url}`, format };

+    if (this._resolve !== defaultResolve) {
+      // throws ERR_INVALID_URL for resolve if not valid


I think here's one of the places we can use a more descriptive error message. Something like "invalid URL returned by loader" is preferred over the user having to research why an error is thrown.

stack trace should be enough right?

I don't think so in this case, because it's not clear what the call hierarchy is. For example, if it's coming from the URL constructor, then it's pretty obvious. But no one would know what Loader.resolve is without reading the source code.

Sure, I've done some polishing on the exact error messages.

TimothyGu · 2018-06-17T04:35:44Z

test/es-module/test-esm-loader-invalid-url.mjs

+  assert.fail();
+}, (err) => {
+  assert.strictEqual(err.code, 'ERR_INVALID_URL');
+});


I assume we would want .then(common.mustCall()) here?

TimothyGu

A few nits. LGTM functionality-wise.

TimothyGu · 2018-06-17T18:33:51Z

test/es-module/test-esm-loader-invalid-url.mjs

@@ -0,0 +1,12 @@
+// Flags: --experimental-modules --loader ./test/fixtures/es-module-loaders/loader-invalid-url.mjs
+/* eslint-disable node-core/required-modules */


Can we move the import common to before assert to get rid of this override?

TimothyGu · 2018-06-17T18:34:25Z

test/fixtures/es-module-loaders/loader-invalid-url.mjs

@@ -0,0 +1,9 @@
+export async function resolve(specifier, parentModuleURL, defaultResolve) {
+  if (parentModuleURL && specifier !== 'assert') {


Do we have to exclude ../common as well?

BridgeAR

I wonder if overloading the ERR_INVALID_RETURN_VALUE would be best for all use cases here. And another test to check the error in case of a returned primitive would be nice as well.

BridgeAR · 2018-06-18T09:42:14Z

test/common/index.mjs

+import common from './index.js';
+
+const { mustCall } = common;
+export { mustCall };



If ./index.js is imported, the already existing functionality here should be removed as that would be duplicated.

I suggest to export all common functions and to remove everything else from this file. I personally would move that code change to a individual commit.

BridgeAR · 2018-06-18T09:45:47Z

test/es-module/test-esm-loader-invalid-url.mjs

+  assert.fail();
+}, (err) => {
+  assert.strictEqual(err.code, 'ERR_INVALID_RETURN_PROPERTY_STRING');
+})


It would be nice to use common.expectsError here instead of the second function. That way the error message itself could also be verified.

The code could look like e.g.

.then( assert.fail, common.expectsError({ code: 'ERR_INVALID_RETURN_PROPERTY_STRING', message: '...' }) ).then(mustCall());

BridgeAR · 2018-06-18T09:49:18Z

lib/internal/errors.js

-  return `Expected ${input} to be returned from the "${name}"` +
-          ` function but got ${type}.`;
+  return `Expected ${input} to be returned from the ` +
+         `"${name}" function but got ${type}.`;


Nit: this is a unrelated change.

BridgeAR · 2018-06-18T09:52:40Z

lib/internal/errors.js

+}, TypeError);
+E('ERR_INVALID_RETURN_PROPERTY_STRING', (input, name, prop, value) => {
+  return `Expected a valid ${input} to be returned for the ${prop} from the ` +
+         `"${name}" function but got ${value}.`;


Nit: If this error code should still be kept, I suggest to change it to a string by reordering the arguments.

Because this is quite a complex arguments case, I think it is important to have the labelled arguments and also the order is important to the message.

I already thought so but all other errors work like that and it should still be relatively clear what it is about.

BridgeAR · 2018-06-18T10:02:50Z

lib/internal/modules/esm/loader.js

@@ -56,16 +58,32 @@ class Loader {
      await this._resolve(specifier, parentURL, defaultResolve);


It might be useful to validate that the return type is actually an object. Otherwise the following error messages would still be weird (e.g., in case 5 is returned the error message would be about the url property).

Sure, I didn't realise destructuring didn't throw for such cases actually.

BridgeAR · 2018-06-18T10:03:41Z

lib/internal/modules/esm/loader.js

-      throw new ERR_INVALID_ARG_TYPE('format', 'string', format);
+      throw new ERR_INVALID_RETURN_PROPERTY_STRING(
+        'string', 'loader resolve', 'format', format
+      );


This error should likely be ERR_INVALID_RETURN_PROPERTY just as the error above. Or I fail to see why this is handled differently than the url error case.

The first error checks that it is a string, the second error checks that it is the right type of string.

The messages are:

Expected string to be returned for the url from the "loader resolve" function but got ${type}.

Expected a valid url to be returned for the url from the "loader resolve" function but got ${value}.

I fail to see the second case being related to this error. It is about format?

Ahh, I missed this was specifically the format case, but the same argument applied - format must be one of a specific set of string values, so the error message will say:

Expected a valid format to be returned for the format from the "loader resolve" function but got ${value}

instead of

Expected string to be returned for the format from the "loader resolve" function but got string.

BridgeAR · 2018-06-18T10:11:42Z

doc/api/errors.md

+### ERR_INVALID_RETURN_PROPERTY_STRING
+
+Thrown in case a function option does not return an expected string property
+type.


I am not sure why strings got special handled while other types not. I recommend to change this to: ERR_INVALID_RETURN_PROPERTY_VALUE.
The current description is definitely wrong as the usage is not about the returned type but about the returned value (at least when looking at the actual implementation).

Agreed on the semantics, but the thing is ideally the current ERR_INVALID_RETURN_VALUE error should then be ERR_INVALID_RETURN_TYPE as that is what it is really checking. So I was trying to remain consistent with that. I think ERR_INVALID_RETURN_PROPERTY_VALUE isn't suitable for this reason. type in the description is referring to "types of strings" as in categories / enums.

I agree that the ERR_INVALID_RETURN_VALUE could be improved. However the return checks are not common and could be consolidated to always be a ERR_INVALID_RETURN_VALUE, just with a more specific error message. IMHO differentiating the type error from the value error is not useful for users.

That all aside: I am definitely against having a error code that is specific for property strings. So at least that has to be more generalized out of my perspective.

IMHO differentiating the type error from the value error is not useful for users.

See my example response in the last comment for why I definitely feel this is a useful distinction in the error message.

In terms of naming, I've switched them around to use ERR_INVALID_RETURN_PROPERTY_VALUE to be consistent with return value, then made the new one just ERR_INVALID_RETURN_PROPERTY.

BridgeAR · 2018-06-18T10:12:02Z

doc/api/errors.md

@@ -1204,6 +1204,17 @@ An invalid `options.protocol` was passed.
 Both `breakEvalOnSigint` and `eval` options were set in the REPL config, which
 is not supported.

+<a id="ERR_INVALID_RETURN_PROPERTY"></a>
+### ERR_INVALID_RETURN_PROPERTY


The name here might be better as: ERR_INVALID_RETURN_PROPERTY_TYPE?

Again, the consistent name here would be ERR_INVALID_RETURN_PROPERTY_VALUE actually :)

BridgeAR · 2018-06-18T12:32:09Z

lib/internal/errors.js

+}, TypeError);
+E('ERR_INVALID_RETURN_PROPERTY_STRING', (input, name, prop, value) => {
+  return `Expected a valid ${input} to be returned for the ${prop} from the ` +
+         `"${name}" function but got ${value}.`;


I already thought so but all other errors work like that and it should still be relatively clear what it is about.

BridgeAR · 2018-06-18T12:33:49Z

lib/internal/modules/esm/loader.js

-      throw new ERR_INVALID_ARG_TYPE('format', 'string', format);
+      throw new ERR_INVALID_RETURN_PROPERTY_STRING(
+        'string', 'loader resolve', 'format', format
+      );


I fail to see the second case being related to this error. It is about format?

BridgeAR · 2018-06-18T12:34:35Z

test/common/index.mjs

+  restoreStdout,
+  restoreStderr,
+  isCPPSymbolsNotMapped
+} = common;


It should be sufficient to just export common, right?

We can only do this once we have named exports support for CommonJS modules.

BridgeAR · 2018-06-18T17:08:16Z

doc/api/errors.md

+### ERR_INVALID_RETURN_PROPERTY_STRING
+
+Thrown in case a function option does not return an expected string property
+type.


I agree that the ERR_INVALID_RETURN_VALUE could be improved. However the return checks are not common and could be consolidated to always be a ERR_INVALID_RETURN_VALUE, just with a more specific error message. IMHO differentiating the type error from the value error is not useful for users.

That all aside: I am definitely against having a error code that is specific for property strings. So at least that has to be more generalized out of my perspective.

guybedford · 2018-06-21T09:37:38Z

CI: https://ci.nodejs.org/job/node-test-pull-request/15545/

guybedford · 2018-06-21T10:58:38Z

@BridgeAR let me know if you have further feedback on the error cases, otherwise could you mark your review on the error adjustments here as approved?

guybedford · 2018-06-22T17:55:28Z

Will merge this Monday / Tuesday if there is no further feedback.

BridgeAR

I would like to get some further tests in to actually check for all the new error cases before this lands.

BridgeAR · 2018-06-23T17:29:31Z

lib/internal/modules/esm/loader.js

-      throw new ERR_INVALID_ARG_TYPE('format', 'string', format);
+      throw new ERR_INVALID_RETURN_PROPERTY(
+        'module format', 'loader resolve', 'format', format
+      );


I am still not certain why typeof format !== 'string' is an ERR_INVALID_RETURN_PROPERTY while typeof url !== 'string' is an ERR_INVALID_RETURN_PROPERTY_VALUE. The validation step seems the same so it is somewhat weird for me that it results in different errors.

guybedford · 2018-06-26T10:38:03Z

@BridgeAR thanks, I've corrected the inconsistency there and added a test. Would be good to get this in soon if you can confirm a final review.

guybedford · 2018-06-26T10:38:41Z

CI: https://ci.nodejs.org/job/node-test-pull-request/15630/

jasnell · 2018-06-29T19:00:34Z

Failure on the one linux bot appears unrelated, but just to be safe: https://ci.nodejs.org/job/node-test-commit-linux/19847/

(am I doing it right @Trott ;-P ...)

Trott · 2018-06-29T20:42:15Z

(am I doing it right @Trott ;-P ...)

That link seems to be to a CI job I started, not one that you restarted.

In this particular case, I'd go to the node-test-pull-request job and use "Resume Build". That will create a new node-test-pull-request job with all the green stuff preserved (no re-run of it) but anything else (yellow, red, grey) re-run.

I just did that, and here's the resulting node-test-pull-request:

https://ci.nodejs.org/job/node-test-pull-request/15673/

PR-URL: #21352 Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

guybedford · 2018-06-30T00:10:56Z

@jasnell thanks for the bump on this, and nice to know the trick to get CI green there.

I went ahead and merged in 1bf42f4.

targos · 2018-06-30T09:20:06Z

This lands cleanly on v10.x-staging but the test fails.

Maybe it depends on a previous change that wasn't backported yet? See https://github.com/nodejs/node/issues?q=label%3Abackport-requested-v10.x+is%3Aclosed+sort%3Aupdated-desc for the list of PRs waiting for a backport.

=== release test-esm-loader-invalid-format ===
Path: es-module/test-esm-loader-invalid-format
Mismatched noop function calls. Expected exactly 1, actual 0.
    at exports.mustCall (/home/mzasso/git/nodejs/v10.x/test/common/index.js:428:10)
    at file:///home/mzasso/git/nodejs/v10.x/test/es-module/test-esm-loader-invalid-format.mjs:11:7
    at ModuleJob.run (internal/modules/esm/module_job.js:96:12)
(node:1657) ExperimentalWarning: The ESM module loader is experimental.
(node:1657) UnhandledPromiseRejectionWarning: AssertionError [ERR_ASSERTION]: Input A expected to strictly equal input B:
+ expected - actual

  Comparison {
-   code: 'ERR_INVALID_RETURN_PROPERTY_VALUE',
-   message: 'Expected string to be returned for the "format" from the "loader resolve" function but got type undefined.'
+   code: 'ERR_INVALID_RETURN_PROPERTY',
+   message: 'Expected string to be returned for the "url" from the "loader resolve" function but got "undefined"'
  }
    at new AssertionError (internal/assert.js:268:11)
    at innerFn (/home/mzasso/git/nodejs/v10.x/test/common/index.js:752:15)
    at /home/mzasso/git/nodejs/v10.x/test/common/index.js:468:15
(node:1657) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 1)
(node:1657) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.
Command: out/Release/node --experimental-modules --loader ./test/fixtures/es-module-loaders/loader-invalid-format.mjs /home/mzasso/git/nodejs/v10.x/test/es-module/test-esm-loader-invalid-format.mjs

targos · 2018-06-30T09:40:22Z

This is actually broken on master:
https://github.com/nodejs/node/runs/5893849
https://ci.nodejs.org/job/node-test-pull-request/15680/

The previously landed commit was broken and it’s too late to force-push. Fixing up the test seems to work. Refs: nodejs#21352

The previously landed commit was broken and it’s too late to force-push. Fixing up the test seems to work. Refs: #21352 PR-URL: #21605 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

PR-URL: #21352 Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

The previously landed commit was broken and it’s too late to force-push. Fixing up the test seems to work. Refs: #21352 PR-URL: #21605 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

devsnek reviewed Jun 15, 2018

View reviewed changes

devsnek approved these changes Jun 15, 2018

View reviewed changes

jasnell requested changes Jun 15, 2018

View reviewed changes

jasnell approved these changes Jun 15, 2018

View reviewed changes

TimothyGu reviewed Jun 17, 2018

View reviewed changes

guybedford force-pushed the loader-url-validate branch 3 times, most recently from 65f89b7 to ea909c3 Compare June 17, 2018 14:37

TimothyGu approved these changes Jun 17, 2018

View reviewed changes

guybedford force-pushed the loader-url-validate branch from 830956a to f4e6acf Compare June 17, 2018 18:53

BridgeAR reviewed Jun 18, 2018

View reviewed changes

guybedford added 5 commits June 18, 2018 14:00

refactor common

bc83382

esm: validate custom loader url

cf1e32f

braces nit

6594462

remove unnecessary try wrapper

1776edf

beautiful error messages

888d186

guybedford force-pushed the loader-url-validate branch from f4e6acf to 3bece0a Compare June 18, 2018 12:16

error handling feedback

233cf2a

guybedford force-pushed the loader-url-validate branch from 3bece0a to 233cf2a Compare June 18, 2018 12:25

BridgeAR reviewed Jun 18, 2018

View reviewed changes

guybedford force-pushed the loader-url-validate branch from 30f9ab0 to babc084 Compare June 18, 2018 21:51

test case refinements

d434368

guybedford force-pushed the loader-url-validate branch from babc084 to d434368 Compare June 18, 2018 21:55

targos mentioned this pull request Jun 24, 2018

test: add test for missing dynamic instantiate hook #21506

Closed

3 tasks

BridgeAR reviewed Jun 24, 2018

View reviewed changes

format case test and fix

a235f08

guybedford force-pushed the loader-url-validate branch from 9607303 to a235f08 Compare June 26, 2018 10:37

guybedford added a commit that referenced this pull request Jun 30, 2018

esm: loader hook URL validation and error messages

1bf42f4

PR-URL: #21352 Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

guybedford closed this Jun 30, 2018

targos added esm Issues and PRs related to the ECMAScript Modules implementation. test Issues and PRs related to the tests. backport-requested-v10.x labels Jun 30, 2018

targos removed the backport-requested-v10.x label Jun 30, 2018

addaleax added a commit to addaleax/node that referenced this pull request Jun 30, 2018

test: fix ESM test

5b32ea9

The previously landed commit was broken and it’s too late to force-push. Fixing up the test seems to work. Refs: nodejs#21352

addaleax mentioned this pull request Jun 30, 2018

test: fix ESM test [Unbreak master] #21605

Closed

targos pushed a commit that referenced this pull request Jun 30, 2018

esm: loader hook URL validation and error messages

29299cc

PR-URL: #21352 Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

targos mentioned this pull request Jul 3, 2018

v10.6.0 proposal #21629

Merged

		@@ -0,0 +1,8 @@
		export async function resolve(specifier, parentModuleURL, defaultResolve) {
		if (parentModuleURL && specifier !== 'assert')

		@@ -0,0 +1,12 @@
		// Flags: --experimental-modules --loader ./test/fixtures/es-module-loaders/loader-invalid-url.mjs
		/* eslint-disable node-core/required-modules */

		@@ -0,0 +1,9 @@
		export async function resolve(specifier, parentModuleURL, defaultResolve) {
		if (parentModuleURL && specifier !== 'assert') {

		@@ -56,16 +58,32 @@ class Loader {
		await this._resolve(specifier, parentURL, defaultResolve);

esm: Ensure custom loader resolved "url" is properly validated #21352

esm: Ensure custom loader resolved "url" is properly validated #21352

Conversation

guybedford commented Jun 15, 2018

Checklist

nodejs-github-bot commented Jun 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guybedford commented Jun 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimothyGu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guybedford Jun 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guybedford commented Jun 21, 2018

guybedford commented Jun 21, 2018

guybedford commented Jun 22, 2018

BridgeAR left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guybedford commented Jun 26, 2018

guybedford commented Jun 26, 2018

jasnell commented Jun 29, 2018

Trott commented Jun 29, 2018

guybedford commented Jun 30, 2018

targos commented Jun 30, 2018

targos commented Jun 30, 2018

guybedford Jun 18, 2018 •

edited

Loading